PC-965 Fixes highlighting when inline HTML tags are present in Classic editor and in Shopify#19162
Conversation
| /** | ||
| * In 'markup', we apply the markings also inside the anchor's attribute if there is a match, on top of | ||
| * marking the anchor's text. | ||
| * The step below is to replace the incorrectly marked anchors with the marked anchors that we want: | ||
| * where the markings are only applied in the anchor's text. | ||
| */ | ||
| if ( anchors.length > 0 ) { | ||
| const markupAnchors = getAnchorsFromText( markup ); | ||
| for ( let i = 0; i < markupAnchors.length; i++ ) { | ||
| markup = markup.replace( markupAnchors[ i ], markedAnchors[ i ] ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The approach is as follows:
- Get the anchors from the sentence
- For every anchor, apply the markings only to the anchor text. Replace the unmarked anchor in the sentence with the marked anchor:
- Retrieve the anchor text
- Apply the marking to the anchor text
- Replace the original anchor text with the marked anchor text
- If there is an anchor found in the sentence:
- The incorrectly marked anchor will be replaced with the correctly marked anchor (only marked anchor text, excluding the attributes), retrieved from
getMarkedAnchors()
- The incorrectly marked anchor will be replaced with the correctly marked anchor (only marked anchor text, excluding the attributes), retrieved from
This approach works, but I have to admit that it's a little bit convoluted.
Let me know if you have suggestions on how to improve the approach :)
There was a problem hiding this comment.
@agnieszkaszuba 's suggestion for improvement for this approach:
- Before running the functionality in
addMarkSingleWord, get all the<a>tags (so only the opening tags) from the text and save them in an array (you can use this part of the regex fromgetAnchorsFromText:<a[\s]+(?:[^>]+)>) - Apply marks to the text (
addMarkSingleWord) - Replace the
<a>tags in the marked text with the original<a>tags
The advantage of this approach is that we don’t need to mark the text between alt tags twice, like it’s currently done in addMarkSingleWord and in getMarkedAnchors.
There was a problem hiding this comment.
We tried to implement the above suggestion. However, the third step "Replace the tags in the marked text with the original tags" proved to be tricky. And we couldn't formulate a regex that can detect anchor opening tag that includes the <yoastmark>.
Hence, we decided to use the current approach.
| mark._properties.marked = languageProcessing.replaceSingleQuotesInTags( mark._properties.marked ); | ||
| mark._properties.original = languageProcessing.replaceSingleQuotesInTags( mark._properties.original ); | ||
|
|
There was a problem hiding this comment.
Classic editor uses double quotes for HTML attribute values. However, in yoastseo, we use single quotes for the attribute values when we create the marked object. As the result, the replacement did not work, as the marks passed by yoastseo did not match anything in the original text.
This step is replacing the single quotes in the marked object output by yoastseo with double quotes. This way, we make sure that the replacement can find a match between the original text of the marked object and the text in the page.
There was a problem hiding this comment.
In yoastseo, single quotes are used because Block editor uses single quotes for HTML tag attributes.
'topic' is an umbrella term for keyphrase and synonyms that we use in the code. But the functionality in this file is also used to mark words in the word complexity assessment, so the using the word 'topic' is no longer accurate here
…ast-markers-break-html-of-content # Conflicts: # packages/js/src/decorator/tinyMCE.js
|
CR: 💯 |
…om:Yoast/wordpress-seo into PC-965-yoast-markers-break-html-of-content # Conflicts: # packages/yoastseo/src/languageProcessing/helpers/word/markWordsInSentences.js
|
CR: My most sincere compliments for accomplishing such a tricky change! 👏 |
12d81ce to
deef450
Compare
|
During acceptance testing, a bug was found. See video. Screen.Recording.2022-11-23.at.15.02.55.mov |
…C-965-yoast-markers-break-html-of-content
…C-965-yoast-markers-break-html-of-content
|
Acceptance testing passed with flying colours 🎉 |
Context
Summary
This PR can be summarized in the following changelog entry:
yoastmarktags broke the HTML when applied to inline HTML attributes.yoastmarkto anchor tag attributes.Relevant technical choices:
anchorHTML tags, since the reported bug is related only to these tags.blockquote,embed,img,linktopicin themarkWordsInSentencesfile were changed towords.Topicis an umbrella term for keyphrase and synonyms that we use in the code. But the functionality in this file is also used to mark words in the word complexity assessment, so the using the word 'topic' is no longer accurate there.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
WordPress
catnip.txt
NOTE: Add the text in
Texteditor mode in Classic editor, and in Code editor mode in Block editorKeyword density assessment
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Keyphrase distribution assessment
composer require yoast/wordpress-seo:dev-PC-965-yoast-markers-break-html-of-content@devbefore building ithrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Word complexity assessment
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Sentence length, Passive voice, and Transition words assessment
However, the compounds were found to repel <a href="https://en.wikipedia.org/wiki/Mosquito" rel="nofollow">mosquitos</a>, and it is hypothesized that rubbing against the plants provides the cats with a chemical coat that protects them against mosquito bites.NOTE: Add the text in
Texteditor mode in Classic editor, and in Code editor mode in Block editorConsecutive sentences assessment
<a href="https://en.wikipedia.org/wiki/Cat">Cats</a> detect nepetalactone through their <a href="https://en.wikipedia.org/wiki/Olfactory_epithelium">olfactory epithelium</a>, not through their vomeronasal organ. <a href="https://en.wikipedia.org/wiki/Cat">Cats</a> detect nepetalactone through their <a href="https://en.wikipedia.org/wiki/Olfactory_epithelium">olfactory epithelium</a>, not through their vomeronasal organ. <a href="https://en.wikipedia.org/wiki/Cat">Cats</a> detect nepetalactone through their <a href="https://en.wikipedia.org/wiki/Olfactory_epithelium">olfactory epithelium</a>, not through their vomeronasal organ.NOTE: Add the text in
Texteditor mode in Classic editor, and in Code editor mode in Block editorParagraph length assessment
Inclusive language analysis
Upgrade routine
Install and activate the previous version of Yoast SEO
Set the site language to English
Create a post in Classic editor and add this text:
catnip.txt
NOTE: Add the text in
Texteditor mode in Classic editorSet "catnip" as the focus keyphrase
Embed this link below to the phrase "catnip flowers":
Test in Shopify
Install and activate Yoast SEO for Shopify
maininshopify-seowordpress-seobefore buildingSet the store language to English
Create a product and add this text:
catnip.txt
NOTE: the test instruction below should be repeated in all content types in Shopify
Keyword density and Keyphrase distribution assessment
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Confirm that the keyword density assessment still detects 8 occurrences of the keyphrase in the text
Click the eye icon of keyword density assessment
Confirm that the "catnip" in "catnip flower" is highlighted
Check the anchor link of the phrase and confirm that the
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Click the eye icon of keyphrase distribution assessment
Confirm that the "catnip" in "catnip flowers" is highlighted
Check the anchor link of the phrase and confirm that the
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Word complexity assessment
hrefvalue doesn't contain yoast mark tags, e.g."<yoastmark class='yoast-text-mark'>"or"</yoastmark>"Relevant test scenarios
composer require yoast/wordpress-seo:dev-PC-965-yoast-markers-break-html-of-content@dev --devbefore building itTest instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
anchorHTML tags, since the reported bug is related to only that tags.blockquote,img,embed,iframe.anchorHTML tags, since the reported bug is related to only that tags.blockquote,img,embed,iframe.<img src="https://www.allaboutbirds.org/guide/assets/photo/308074031-480px.jpg" alt="Photos and Videos for Rock Pigeon, All About Birds, Cornell ..." /><iframe src="demo_iframe.htm" height="200" width="300" title="Iframe Example"></iframe>UI changes
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.Documentation
Quality assurance
Fixes https://yoast.atlassian.net/browse/PC-965 and https://yoast.atlassian.net/browse/IM-2081